Table #343

bcampbell · 2018-04-20T20:13:49Z

(see #310)
For unix/darwin/windows, this adds support for:

single or multi-select (set at creation time)
an OnSelectionChanged callback
an interface for iterating through the set of selected items
a noddy example app (examples/table)

The windows version supports only text columns (being based on the rather limited win32 commctl listview).

also: fixed column-ordering bug changed file to hard tabs

Still a lot of features to exercise, but now provides some generated, multi-column data to work with.

windows expects a string to persist between WM_NOTIFY messages, so I've added one to the uiTable struct to keep it around and to clean up upon when the table is destroyed.

In LVN_GETDISPINFO, it turns out you can just fill out a provided text buffer rather than all the craziness involved with allocating a new one and making sure it hangs around long enough. Phew.

- enable full-row selection (instead of individual cell) - show hover tooltip when content too large for cell

mostly just stubs so far, barring some GTK+ support for multiselect

windows and mac just stubbed for now

Added a param to uiNewTable for passing in creation-time style flags, as we can't guarentee that all platforms support will support changing styles and behaviours on the fly. I'm looking at you, windows.

mischnic · 2018-04-20T20:21:32Z

In examples/CMakeLists.txt:40 inside if(APPLE) you should add

target_compile_options(table PRIVATE --stdlib=libc++)
target_link_libraries(table --stdlib=libc++)

At the moment, this is to silence a warning, but if you were to use some C++11 feature there would be a compile error.

andlabs · 2018-04-20T20:49:30Z

I thought I did that already. If I didn't, there's discussion about this somewhere that I'd want to point to in comments around those two lines... and it should go in its own commit with its own commit message (regardless of PR).

bcampbell · 2018-04-20T22:00:45Z

I'm guessing the relevant discussion is #302.
Anyway, it's an issue again because the new table example I added is in C++. Sorry.
@mischnic: I've patched up the examples/CMakelists.txt appropriately, but I've not tested it on a Mac yet - care to give it another go?

andlabs · 2018-04-21T00:25:18Z

TABLE_NOTES.md

+I think a single uiTableModelAllChanged() function would be
+enough. The underlying control needs to know that it should
+to redraw everything visible (and the number of items might
+have chnaged).


As noted, the problem is GTK+ does not seem to provide a way to do this. There might be workarounds that are inefficient (requiring a signal emission for every row of the table, for instance), and I'm not sure if removing and re-adding the model will reset other settings like column widths and scroll positions. Will need to ask the GTK+ people about it.

Might want to note this here. Not required; not part of review.

andlabs · 2018-04-21T00:57:30Z

All right. I appreciate everything being done here, but I decided to abandon my initial code review for two big reasons: a) the code formatting and style is very inconsistent with the rest of libui, so the review would have been flooded with stylistic consistency nitpicks, and I don't want to come off looking like a pedantic jerk, and b) and more important, this PR pushes in too many changes at once that I'm not fully comfortable with to just take in all at once, and we'd just be dragging this along forever if we discussed everything now all at once.

It's my fault for not realizing just how much was going into this PR; sorry about that. But I don't want to reject it outright. Could we possibly split it into smaller PRs that add things a feature at a time? I can merge the table branch into master without the extra features, and then add the features back in with later CLs; no interim binary release necessary, especially since Alpha 4 is still not ready for primetime even with the basic uiTable stuff in.

(I will say that your Mac code is definitely wrong — your tableDelegateClass overrides the existing NSTreeViewDelegate that comes from uiTableModel itself, and the tables won't work. I can take care of fixing all of that if you want as well, which is another reason to split the PR into smaller ones.)

(I really need to find a way to nicely explain that libui code should also look the same as all the other code. I should investigate things like clang-format, maybe...)

bcampbell · 2018-04-21T01:57:50Z

Sure - I'm happy to split it up into more easily-digestible chunks (and you can nit-pick them at leisure ;- )

I guess the rough plan would be:

add a windows implementation to the current table branch (barring non-text part support)
add the various features back in one at a time (reading the selection, OnSelectionChange, multiselection mode, example app etc)

Sound OK to you?

andlabs · 2018-04-21T02:58:46Z

Yeah. In fact, if the features are not interdependent on each other, we wouldn't have to go in a strict order either. I would do one feature first, though, just to get the general interface for features out of the way. Thanks again!

mischnic · 2018-04-21T09:51:47Z

I've patched up but I've not tested it on a Mac yet - care to give it another go?

👍

The test crashes on macOS when selecting a different row:

$ ./out/test
2018-04-21 11:50:49.341 test[5045:322334] get 0x0
main window 0x7ffe21426320
size
Segmentation fault: 11

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY

Termination Signal:    Segmentation fault: 11
Termination Reason:    Namespace SIGNAL, Code 0xb
Terminating Process:   exc handler [0]

VM Regions Near 0:
--> 
    __TEXT                 000000010def6000-000000010df0c000 [   88K] r-x/rwx SM=COW  /Users/USER/Desktop/*

Application Specific Information:
Performing @selector(tableViewSelectionDidChange:) from sender tableView 0x7ffe23e1cb70

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   ???                           	000000000000000000 0 + 0
1   libui.A.dylib                 	0x000000010df516ef -[tableDelegateClass tableViewSelectionDidChange:] + 79 (table.m:102)
2   libsystem_trace.dylib         	0x00007fffa3f033a7 _os_activity_initiate_impl + 53
3   com.apple.AppKit              	0x00007fff8c377721 -[NSApplication(NSResponder) sendAction:to:from:] + 456
4   libui.A.dylib                 	0x000000010df4733f -[applicationClass sendAction:to:from:] + 143 (main.m:32)
5   com.apple.AppKit              	0x00007fff8be5bcc4 -[NSControl sendAction:to:] + 86
6   com.apple.AppKit              	0x00007fff8bed2d10 -[NSTableView _sendAction:to:row:column:] + 111
7   com.apple.AppKit              	0x00007fff8bed150f -[NSTableView mouseDown:] + 7439
8   com.apple.AppKit              	0x00007fff8c4f324f -[NSWindow(NSEventRouting) _handleMouseDownEvent:isDelayedEvent:] + 6341
9   com.apple.AppKit              	0x00007fff8c4efa6c -[NSWindow(NSEventRouting) _reallySendEvent:isDelayedEvent:] + 1942
10  com.apple.AppKit              	0x00007fff8c4eef0a -[NSWindow(NSEventRouting) sendEvent:] + 541
11  com.apple.AppKit              	0x00007fff8c373681 -[NSApplication(NSEvent) sendEvent:] + 1145
12  libui.A.dylib                 	0x000000010df472a2 -[applicationClass sendEvent:] + 82 (main.m:20)
13  com.apple.AppKit              	0x00007fff8bbee427 -[NSApplication run] + 1002
14  libui.A.dylib                 	0x000000010df4792b uiMain + 43 (main.m:165)
15  test                          	0x000000010defd568 main + 1416 (main.c:170)
16  libdyld.dylib                 	0x00007fffa3cd1235 start + 1

bcampbell · 2018-04-21T10:32:51Z

Thanks, @mischnic
I probably won't get a chance to check this out for a while, but I imagine this could be related to whatever I did wrong with NSTreeViewDelegate. The table example did work fine on my mac, but it's a few versions of OSX behind, so all bets are off.

andlabs · 2018-04-29T00:46:50Z

Is there an ETA on the new PR? I want to merge it before continuing my large-scale uipriv- prefix changes...

bcampbell · 2018-04-29T10:14:38Z

I'll aim to get the bare-bones comctl win32 implementation up for you in the next few days (ie no API changes, just adding windows support to the base table branch)

It'll incorporate #287, too, I'm afraid, as I need that to compile. I assume you can skip those commits in the PR, but let me know if there's a more elegant approach.

andlabs · 2018-05-07T00:55:47Z

Superseded by #361 (and future PRs from there), especially now that I handled #287 separately

bcampbell added 23 commits February 10, 2017 15:53

add table example

18d9aab

oops. table example now works.

3c8ada2

Merge remote-tracking branch 'upstream/table' into table

aef5f88

add bare-bones table support for win32 (using comctl listview)

62ca6d1

windows comctl table: notify listview upon model changes

fc5df3c

also: fixed column-ordering bug changed file to hard tabs

table example: switch to C++, generate dummy data

24b7f3e

Still a lot of features to exercise, but now provides some generated, multi-column data to work with.

win32 comctl table: use std::vector instead of hand-rolled

b81111b

windows comctl table: tidy up WM_NOTIFY string hack

cff67d5

windows expects a string to persist between WM_NOTIFY messages, so I've added one to the uiTable struct to keep it around and to clean up upon when the table is destroyed.

windows comctl table: win32 unexpectedly less bonkers than feared

620bfd2

In LVN_GETDISPINFO, it turns out you can just fill out a provided text buffer rather than all the craziness involved with allocating a new one and making sure it hangs around long enough. Phew.

windows comctl table: nicer default style

067d605

- enable full-row selection (instead of individual cell) - show hover tooltip when content too large for cell

add my notes on table API and implementation

6547cc9

table: add style flags and onSelectionChanged

61e1cc2

mostly just stubs so far, barring some GTK+ support for multiselect

table (osx): support multiselect style flag

b5ed561

refine table-selection notes across the platforms

de88440

table: add access to current selection (gtk code)

5be3285

windows and mac just stubbed for now

windows: support uiTable OnSelectionChanged callback

e13d11e

windows: uiTable support for selection iteration

4870305

table: move style flags into creation

f75758e

Added a param to uiNewTable for passing in creation-time style flags, as we can't guarentee that all platforms support will support changing styles and behaviours on the fly. I'm looking at you, windows.

mac: implement uiTableGetSelection and uiTableIter

11a98bf

tables: add onSelectionChanged support to OSX version

0e56638

table example: add delete button

a9bacb2

Merge remote-tracking branch 'upstream/table' into table

6953267

table: update ui[Free|Alloc] => uipriv[Free|Alloc]

c32baa4

bcampbell added 2 commits April 21, 2018 09:48

update test for new uiNewTable

d70e977

apple-specific c++ build kludge for table example

5f96010

andlabs reviewed Apr 21, 2018

View reviewed changes

mischnic mentioned this pull request May 4, 2018

Lists ? kusti8/proton-native#99

Closed

andlabs closed this May 7, 2018

bcampbell deleted the table branch July 25, 2018 23:10

benbucksch mentioned this pull request Dec 7, 2019

Feature: single select and multi select list items kusti8/proton-native#202

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Table #343

Table #343

bcampbell commented Apr 20, 2018 •

edited

Loading

mischnic commented Apr 20, 2018 •

edited

Loading

andlabs commented Apr 20, 2018 •

edited

Loading

bcampbell commented Apr 20, 2018

andlabs Apr 21, 2018 •

edited

Loading

andlabs commented Apr 21, 2018

bcampbell commented Apr 21, 2018

andlabs commented Apr 21, 2018 •

edited

Loading

mischnic commented Apr 21, 2018 •

edited

Loading

bcampbell commented Apr 21, 2018

andlabs commented Apr 29, 2018

bcampbell commented Apr 29, 2018

andlabs commented May 7, 2018

Table #343

Table #343

Conversation

bcampbell commented Apr 20, 2018 • edited Loading

mischnic commented Apr 20, 2018 • edited Loading

andlabs commented Apr 20, 2018 • edited Loading

bcampbell commented Apr 20, 2018

andlabs Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

andlabs commented Apr 21, 2018

bcampbell commented Apr 21, 2018

andlabs commented Apr 21, 2018 • edited Loading

mischnic commented Apr 21, 2018 • edited Loading

bcampbell commented Apr 21, 2018

andlabs commented Apr 29, 2018

bcampbell commented Apr 29, 2018

andlabs commented May 7, 2018

bcampbell commented Apr 20, 2018 •

edited

Loading

mischnic commented Apr 20, 2018 •

edited

Loading

andlabs commented Apr 20, 2018 •

edited

Loading

andlabs Apr 21, 2018 •

edited

Loading

andlabs commented Apr 21, 2018 •

edited

Loading

mischnic commented Apr 21, 2018 •

edited

Loading